Skip to content

Add preview command support - #21

Open
jsines wants to merge 70 commits into
mainfrom
js/hs-preview
Open

Add preview command support#21
jsines wants to merge 70 commits into
mainfrom
js/hs-preview

Conversation

@jsines

@jsines jsines commented Jul 26, 2023

Copy link
Copy Markdown

See PR description for HubSpot/hubspot-cli#898

@jsines
jsines marked this pull request as ready for review October 12, 2023 18:04
Comment thread api/preview.js Outdated
});
}

const fetchContentMetadata = async (

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much of this ripped straight from cms-js-platform

@TanyaScales

Copy link
Copy Markdown
Member

Error: Cannot find module 'express' -- we need to add express as a dependency here in the lib

@TanyaScales

TanyaScales commented Oct 13, 2023

Copy link
Copy Markdown
Member

With the preview command leveraging some of the watch commands logic in some ways -- have you considered refactoring the watch and preview commands to DRY up the code in the lib?

Taking a look at both, its definitely not something that we need to do initially for MVP because I wouldn't want to get too in the weeds with touching the currently working watch command -- however, it would be best if we didn't have these two commands diverge in their "watching/uploading" functionality if one is changed (the local filesystem aspect of it), the other should (seemingly) behave the same to the developer -- with the nuances of each command built into their own separate areas.

@jsines

jsines commented Oct 16, 2023

Copy link
Copy Markdown
Author

Error: Cannot find module 'express' -- we need to add express as a dependency here in the lib

Oops, missed this - I think I have it as a global so it wasn't erroring for me 🥲

With the preview command leveraging some of the watch commands logic in some ways -- have you considered refactoring the watch and preview commands to DRY up the code in the lib?

Definitely something that has crossed my mind. I think for the time being the best approach might be to keep it separate. There's going to be some unique functionality we want to throw in there to synchronize rendering / uploading stuff, and I wasn't certain about how uploading to the preview buffer would look compared to our normal upload (though it's looking like we may be able to use the same method of uploading, just changing the path.

I do think down the line it might make sense to unify them though, or least clean up hs watch with some of the refactoring I've already done in here - just felt out of scope to include that here.

@TanyaScales

Copy link
Copy Markdown
Member

Definitely something that has crossed my mind. I think for the time being the best approach might be to keep it separate. There's going to be some unique functionality we want to throw in there to synchronize rendering / uploading stuff, and I wasn't certain about how uploading to the preview buffer would look compared to our normal upload (though it's looking like we may be able to use the same method of uploading, just changing the path.

I do think down the line it might make sense to unify them though, or least clean up hs watch with some of the refactoring I've already done in here - just felt out of scope to include that here.

Definitely agree -- Waiting to see how this shapes up is best before deciding to refactor is a good call initially. We can follow up on it after MVP -- as there might be more feedback and quick changes that we need to adopt without thinking how that is going to affect the standard watch command.

Comment thread lib/preview.js Outdated
Comment thread lib/preview/proxyPage.js Outdated
Comment thread lib/preview/routes/proxyPathPageResourceRedirect.js Outdated
Comment thread lib/preview/routes/proxyPathPageResourceRedirect.js Outdated
Comment thread lib/preview/shadowDevServer.js Outdated
Comment thread lib/preview/shadowDevServer.js Outdated
Comment thread lib/preview/routes/index.js Outdated
Comment thread lib/preview/routes/index.js Outdated
Comment thread api/domains.js Outdated
Comment thread api/preview.js Outdated
Comment thread lib/preview/previewUtils.js
Comment thread lib/preview/routes/index.js
Comment thread package.json Outdated
Comment thread lib/preview/routes/proxyPageResourceRedirect.js Outdated
@brandenrodgers

brandenrodgers commented Feb 8, 2024

Copy link
Copy Markdown
Contributor

The code in here looks reasonable to me, but tbh it's too much code for me to be able to confidently ✅ without being able to test it. I just noticed your comment over in the CLI PR. I'll give it a try!

We had previously discussed porting this logic into local-dev-lib, but I think we may want to reconsider that and instead work towards the pattern being used by the JS rendering and UI extensions teams. The local-dev-lib library is intended to contain common logic that gets used in many places. Our current pattern for large, single-use features like this dev server is to offer them as their own npm package. Then the CLI can include the package as a dependency. This would give us all a few benefits:

  • Rolling out changes to this package wouldn't require any PR's into the shared libraries (unless it required library changes too)
  • Library owners aren't required to help maintain a large amount of code that they have limited knowledge about
  • Releases of the dedicated package can happen independently of the CLI releases.
  • The UIE (and I think also the JS rendering) dev server offers some basic CLI commands to run the servers outside of the context of the CLI. They can still be marked as dependencies of the CLI, but this enables teams to independently roll out and test changes without being subject to the release flow of the CLI.

Edit: Just to clarify, I don't think doing this ^ should prevent you from being able to merge this PR. We should just chat about it before you get started porting any of this into the local-dev-lib.

@camden11

camden11 commented Feb 8, 2024

Copy link
Copy Markdown
Contributor

Everything's looking good as far as I can tell. I defer to Tanya for all the net new stuff, but as far as backwards compatibility goes I think we're all good

Comment thread lib/preview/routes/module.js Outdated
Comment thread lib/preview/routes/template.js Outdated
} catch (err) {
logger.error(`Failed to fetch module preview for ${calculatedPath}`)
}
if (!customWidgetInfo || !('moduleId' in customWidgetInfo && 'previewKey' in customWidgetInfo)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems very brittle--do we have plans to refactor this in the future?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what you mean by brittle here? I may be too frog-in-boiling-water to see anything iffy 👀

@kemmerle kemmerle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments directly in the code, where I spotted some possible bugs. I also wanted to ask why we've written a proxy server, when there is express middleware that creates proxies in a single line. Do we need specialized functionality? I agree with @brandenrodgers that we should revisit the code and consider releasing this as its own npm package.

@brandenrodgers

Copy link
Copy Markdown
Contributor

I tested locally and things seem to be working! The only (potential) issue is that I saw this error page when clicking on one of my domains:
image

Maybe that's WAD though?

@jsines

jsines commented Feb 9, 2024

Copy link
Copy Markdown
Author

I also wanted to ask why we've written a proxy server, when there is express middleware that creates proxies in a single line. Do we need specialized functionality?

Hadn't considered using an extra middleware for it - there is some special handling that takes place to convert our special URL format into the actual preview call that needs to happen but I see there's a URL builder sort of feature in there - definitely gonna explore whether it's worth moving that over before launch 👍

I tested locally and things seem to be working! The only (potential) issue is that I saw this error page when clicking on one of my domains:

Does that domain / path exist as a page on your portal? We do a straightforward fetch to the URL provided with some added params to get the preview render so if you're getting that error then it means that call failed

@jsines

jsines commented Feb 9, 2024

Copy link
Copy Markdown
Author

Also +1 on making it its own separate package when we do go to release it - definitely large enough that that's warranted 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants